Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common Paint: Add name getter/setter API #1285

Closed
wants to merge 1 commit into from
Closed

common Paint: Add name getter/setter API #1285

wants to merge 1 commit into from

Conversation

JSUYA
Copy link
Member

@JSUYA JSUYA commented Nov 29, 2022

Adds an API that can set/get string type name to tvgPaint.
This can be used when the user needs to traverse the scene tree or find a specific instance.
+) The SVG's "id" attribute is set to name when SVG is loaded.

Issue:

@JSUYA
Copy link
Member Author

JSUYA commented Nov 29, 2022

This is a simple solution for #1276.
I think the size(cost) of tvgPaint can up unnecessarily. and it can not be used universally except in special cases.
What do you think?

@kariem2k
Copy link

kariem2k commented Dec 6, 2022

Maybe a hashed string can be stored instead of the actual string itself? something like int getPaintId(); and now the user can just do if(paint->getPaintId() == "MySVGShape1"_hs) or if(paint->getPaintId() == hash("MySVGShape1")).

@kariem2k
Copy link

kariem2k commented Dec 6, 2022

And if the user wants to have a full blown svg "reflection", he can just use a different svg parser building the tree and use the hashed ids coming from thorgvg paint->getPaintId() for lookup in the svg dom retrieving more detailed info like classes, hrefs, or the actual id string.

The PaintId can be auto generated as well if it is not coming from and svg for consistency (but this is just a suggestion)

@hermet hermet added the feature New feature additions label Dec 8, 2022
@thorvg thorvg deleted a comment from github-actions bot Dec 26, 2023
@hermet hermet closed this Mar 19, 2024
@hermet hermet added the invalid This doesn't seem right label Mar 19, 2024
@hermet hermet reopened this Mar 19, 2024
@hermet hermet removed the invalid This doesn't seem right label Mar 19, 2024
@JSUYA
Copy link
Member Author

JSUYA commented Mar 19, 2024

Rebased PR

Adds an API that can set/get string type name to tvgPaint.
This can be used when the user needs to traverse the scene tree
or find a specific instance.
+) The SVG's "id" attribute is set to name when SVG is loaded.
@JSUYA
Copy link
Member Author

JSUYA commented May 27, 2024

@hermet Do you have any comments for updates? At the time this patch was written, it wasn't a necessary thing. There were comments related to hashing, but I can't pay attention to them for a long time. (If hashing, it can be used more widly, but for now, I think it is over-spec. It can be sufficiently improved in the future.)
Or someone can take this task.

@hermet
Copy link
Member

hermet commented May 27, 2024

@JSUYA I agree with @kariem2k's opinion; ThorVG could save memory in general cases if it has a name with a single reference id. Thus, the usage would be something like this

//Internally encode the string to a hash ID and compare its value.
if (paint->identify(string))  GOTCHAS!
or 
//Acquire the unique id value from the string and compare it. 
//This will be a bit more dangerous at ABI break.
if (paint->id(string) == paint()->id()) GOTCHAS!

Yet my question is whether tvg::Paint needs to return the name as a string or not. If so, how can it be done without any cost. On consideration.

@JSUYA
Copy link
Member Author

JSUYA commented May 27, 2024

Yet my question is whether tvg::Paint needs to return the name as a string or not. If so, how can it be done without any cost. On consideration.

It is not necessary to return it as a string. We can use if(Accessor::hashToString(paint->id()) == "ID") .

@hermet
Copy link
Member

hermet commented May 27, 2024

Accessor::hashToString

If ThorVG should support value recovery, then I guess hashing wouldn't be beneficial.

@hermet
Copy link
Member

hermet commented May 31, 2024

@JSUYA Please have a look at this. I'm not sure about this feature for SVG since there have been no practical requirements so far. It will just make things difficult for the SVG loader without any purpose.

a0b1e50
380b6d5

@JSUYA
Copy link
Member Author

JSUYA commented May 31, 2024

@JSUYA Please have a look at this. I'm not sure about this feature for SVG since there have been no practical requirements so far. It will just make things difficult for the SVG loader without any purpose.

a0b1e50 380b6d5

Oh I've been waiting for your opinion since i think this discussion is not conclusive. I checked the contents of the hermet/type branch.
In my memory, this issue was requested by someone. #1276 But I don't remember
I agree that it is not a necessary feature now.

I'm closing this PR. If a feature related to this(string type return) is needed, it can be discussed again in a new place other than this PR.

@JSUYA JSUYA closed this May 31, 2024
@hermet hermet added the invalid This doesn't seem right label May 31, 2024
@hermet hermet deleted the dev/tag branch May 31, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature additions invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants